Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a separate labelDrawTime option #275

Merged
merged 4 commits into from
Jan 2, 2021

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Dec 9, 2020

Implementation notes:

Since not all annotation types implement labels, the drawLabel method is optional.

If the labelDrawTime option isn't specified, then it checks the individual annotation's drawTime option, then the chart-wide annotation plugin's labelDrawTime option, then the chart-wide annotation plugin's drawTime option. This means that you can't override an individual annotation's drawTime without also affecting its labelDrawTime, but I think it's the more intuitive and backward-compatible behavior.

Since the option should default to drawTime if not set, I did not add it to the defaults object. Please let me know if I've misunderstood how plugins' default options should be implemented.

@joshkel
Copy link
Contributor Author

joshkel commented Dec 9, 2020

This should fix #243 and #94. As part of this, I also draw labels in a separate loop, so it supersedes #161.

Copy link
Collaborator

@stockiNail stockiNail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to use drawTime options into label node instead of labelDrawTime at option level.

README.md Outdated
@@ -30,6 +30,10 @@ To configure the annotations plugin, you can simply add new config options to yo
// See http://www.chartjs.org/docs/#advanced-usage-creating-plugins
drawTime: 'afterDatasetsDraw', // (default)

// Defines when annotations' labels are drawn. Defaults to
// drawTime.
labelDrawTime: 'afterDatasetsDraw', // (default)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshkel may I ask you why to use a property at element level instead of using drawTime property at label node?

}
});
elements.forEach(el => {
if ('drawLabel' in el && (el.options.labelDrawTime || el.options.drawTime || options.labelDrawTime || options.drawTime || caller) === caller) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshkel using drawTime at label level, can the statement be the following?

if ('drawLabel' in el && (el.options.label.drawTime || el.options.drawTime || options.drawTime || caller) === caller) {

@joshkel
Copy link
Contributor Author

joshkel commented Dec 10, 2020

Thanks for the suggestion, @stockiNail. I hadn't considered using a drawTime property at the label node. I do like that better, although I have a question about how it should be designed: Should setting a chart-wide default drawTime for labels be permitted? That would mean that the chart's options.plugins.annotation has a label sub-object with only one usable property. (The line annotation plugin's labels implementation doesn't otherwise pull any label defaults from the chart's options.plugins.annotation - maybe that's worth raising separately, if it's an issue for anyone. Or maybe there's a way to better merge in defaults automatically; I'm still learning my way around the Chart.js APIs.)

I noticed that text annotations were listed under "To-do Items" (and it's probably something that our project will eventually need). I'm guessing that a hypothetical text annotations feature would use the same label sub-object as lines (to keep a consistent interface) and so whatever we decide here would work for that, too.

@stockiNail
Copy link
Collaborator

Should setting a chart-wide default drawTime for labels be permitted? That would mean that the chart's options.plugins.annotation has a label sub-object with only one usable property. (The line annotation plugin's labels implementation doesn't otherwise pull any label defaults from the chart's options.plugins.annotation - maybe that's worth raising separately, if it's an issue for anyone. Or maybe there's a way to better merge in defaults automatically; I'm still learning my way around the Chart.js APIs.)

@joshkel you would like to set the label drawTime default into annotation plugin options in order that all labels of all annotation can inherit from that. In this case the labelDrawTime should be set, agree with you. Do you think this is the good use case?
In this case, forgive because it makes sense to me as well, I didn't think about that case even if usually this decoupled drawing should be an exception, I guess, and then could be configurable for a single annotation.

I noticed that text annotations were listed under "To-do Items" (and it's probably something that our project will eventually need). I'm guessing that a hypothetical text annotations feature would use the same label sub-object as lines (to keep a consistent interface) and so whatever we decide here would work for that, too.

I was thinking about that (also to have label on the box and all other annotation types) and maybe an abstract label class is needed and every annotation could extend and implement special labeling.
This is valid (in my opinion) also for TextAnnotation (not implemented yet).
But I didn't do yet because I'm working on other stuff, sorry.

@stockiNail
Copy link
Collaborator

stockiNail commented Dec 10, 2020

@joshkel apologize, I forgot an important thing on my previous answer.
@kurkle taught me that the annotations are now Chart.js elements and therefore you can configure the annotation (by its type) also in Chart.defaults.elements and/or chart.options.elements, like point, bar, arc or line (default elements of Chart.js).

Therefore if you want to configure all line annotations (but this is valid for all annotation types) with a specific drawTime (and then let's assume to use drawTime for label as well), you can do something like that:

options: {
   elements: {
       lineAnnotation: {
         drawTime: 'beforeDatasetsDraw',
         label:  {
            drawTime: 'afterDraw'
         },
   }
   ....
   plugins: {
      annotation: {
         annotations: {
            myHorizontalLine: {
               type: 'line',
               scaleID: 'y',
               value: -90,
               label: {
                  position: 'center',
                  font: {
                     size: 16,
                  },
                  backgroundColor: 'red',
                  content: 'This is a label',
                  enabled: true
               }
            },
         }
      }
   }
   ...
}

You can do the same into Chart.defaults as well.

In this way you can configure the drawTime default option for all labels of all line annotations.

Unfortunately a piece of code is currently missing into plugin but I think it makes sense to add:

https://github.com/pepstock-org/chartjs-plugin-annotation/blob/a46915f3e409928281f1e0c9d550bc524c1bddc3/src/annotation.js#L136

el.options = merge(Object.create(null), [elType.defaults, chart.options.elements[elType.id], annotation]);

This line is of my branch where the options are calculated differently for your code (when merged and after conflicts resolving, it should work changing that).

In this way you can get the drawTime also into your label as default for all line annotation and then the drawing can use drawTime (instead of labelDrawTime) for the label as well.

If you agree on that (to use drawLine for label and use the elements for default configuration), I can add the "merge" update to my another branch and submit another PR.

@kurkle
Copy link
Member

kurkle commented Dec 18, 2020

I think I like the label.drawTime more. Lets go with that.

I'm planning to provide some utilities for this in the core, no need to write the resolution path in every plugin (and extension):

el.options = merge(Object.create(null), [elType.defaults, chart.options.elements[elType.id], annotation]);

@stockiNail
Copy link
Collaborator

I'm planning to provide some utilities for this in the core, no need to write the resolution path in every plugin (and extension):

Sounds good!

@joshkel
Copy link
Contributor Author

joshkel commented Dec 31, 2020

Apologies for the delayed reply.

I changed to label.drawTime, as suggested, since that definitely seems like a cleaner design.

Do you want me to do anything with merging options now, or wait until other PRs and/or core work?

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the beginPath() and merging README.md with master, this seems good to go.

src/types/line.js Outdated Show resolved Hide resolved
Implementation notes:

Since not all annotation types implement labels, the `drawLabel` method is optional.

If the `labelDrawTime` option isn't specified, then it checks the individual annotation's `drawTime` option, then the chart-wide annotation plugin's `labelDrawTime` option, then the chart-wide annotation plugin's `drawTime` option. This means that you can't override an individual annotation's `drawTime` without also affecting its `labelDrawTime`, but I think it's the more intuitive and backward-compatible behavior.

Since the option should default to `drawTime` if not set, I did not add it to the `defaults` object. Please let me know if I've misunderstood how plugins' default options should be implemented.
As suggested in code review.
@kurkle kurkle merged commit 3fb1e15 into chartjs:master Jan 2, 2021
@joshkel joshkel deleted the label-draw-time branch January 2, 2021 22:00
joshkel added a commit to joshkel/Chart.js that referenced this pull request Jan 8, 2021
From what I understand, if we want to allow registering additional
element options (see [here][1] for an example), then the element options
need to be a top-level interface so that they can be used with
TypeScript's [declaration merging][2].

[1]: chartjs/chartjs-plugin-annotation#275 (comment)
[2]: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
joshkel added a commit to joshkel/Chart.js that referenced this pull request Jan 8, 2021
From what I understand, if we want to allow registering additional element options (see [here][1] for an example), then the element options need to be a top-level interface so that they can be used with TypeScript's [declaration merging][2].

[1]: chartjs/chartjs-plugin-annotation#275 (comment)
[2]: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
etimberg pushed a commit to chartjs/Chart.js that referenced this pull request Jan 30, 2021
From what I understand, if we want to allow registering additional element options (see [here][1] for an example), then the element options need to be a top-level interface so that they can be used with TypeScript's [declaration merging][2].

[1]: chartjs/chartjs-plugin-annotation#275 (comment)
[2]: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants